refactor: offchain reception on entity store#24142
Conversation
…n-reception-on-entity-store
…n-reception-on-entity-store
| }; | ||
|
|
||
| /// Entity type id shared by every offchain message reception entity in the entity store. | ||
| pub(crate) global OFFCHAIN_RECEPTION_ENTITY: Field = sha256_to_field("AZTEC_NR::OFFCHAIN_RECEPTION_ENTITY".as_bytes()); |
There was a problem hiding this comment.
| pub(crate) global OFFCHAIN_RECEPTION_ENTITY: Field = sha256_to_field("AZTEC_NR::OFFCHAIN_RECEPTION_ENTITY".as_bytes()); | |
| pub(crate) global OFFCHAIN_RECEPTION_ENTITY_TYPE_ID: Field = sha256_to_field("AZTEC_NR::OFFCHAIN_RECEPTION_ENTITY".as_bytes()); |
| /// Number of fields produced by serializing a `BoundedVec<Field, MESSAGE_CIPHERTEXT_LEN>`. | ||
| /// | ||
| /// A `BoundedVec` serializes as its storage fields followed by a trailing length field, so the total is | ||
| /// `MESSAGE_CIPHERTEXT_LEN + 1`. | ||
| global SERIALIZED_CIPHERTEXT_LEN: u32 = MESSAGE_CIPHERTEXT_LEN + 1; |
There was a problem hiding this comment.
I don't think you need this? this value doesn't seem to be used anywhere except inside a fn body:
let serialized_ciphertext: [Field; SERIALIZED_CIPHERTEXT_LEN] = self.ciphertext.serialize();and there it should be inferred from the retun type.
| /// Computes the entity id that identifies this message's reception entity in the entity store. | ||
| /// | ||
| /// The id is a poseidon2 hash over the serialized ciphertext fields and the normalized tx hash (0 when absent). | ||
| /// Re-delivering the same `(ciphertext, tx_hash)` pair therefore maps to the same entity, making `receive` | ||
| /// idempotent. The `recipient` and `anchor_block_timestamp` fields are intentionally excluded, so identity depends | ||
| /// only on the message content and its originating transaction. | ||
| fn entity_id(self) -> Field { |
There was a problem hiding this comment.
I don't think trait impls get docstrings, so these don't make it to the docs.
The second paragraph is an implementation detail, so I'd put it in the body. The overall explanation of what the id represents ('all offchain messages, regardless of content') sounds more like it should go in the docs for OffchainMessage, which is the actual entity, alongside an explanation of the entity lifecycle etc.
| /// `MESSAGE_CIPHERTEXT_LEN + 1`. | ||
| global SERIALIZED_CIPHERTEXT_LEN: u32 = MESSAGE_CIPHERTEXT_LEN + 1; | ||
|
|
||
| impl EntityBody for OffchainMessage { |
There was a problem hiding this comment.
I'm not well versed in Rust idioms so I'm not sure what a good pattern here would be, I just find this slightly strange. I'm personally tempted to do impl EntityId for OffchainMessage, but that also doesn't seem fantastic. I'd consider taking a look at Rust crates or something for inspiration.
| let serialized_ciphertext: [Field; SERIALIZED_CIPHERTEXT_LEN] = self.ciphertext.serialize(); | ||
| let mut inputs: [Field; SERIALIZED_CIPHERTEXT_LEN + 1] = [0; SERIALIZED_CIPHERTEXT_LEN + 1]; | ||
| for i in 0..SERIALIZED_CIPHERTEXT_LEN { | ||
| inputs[i] = serialized_ciphertext[i]; | ||
| } | ||
| inputs[SERIALIZED_CIPHERTEXT_LEN] = self.tx_hash.unwrap_or(0); | ||
| poseidon2_hash(inputs) |
There was a problem hiding this comment.
If the goal is to make the id be 1-1 with the content, then we could request that EntityBody implement Serialize, and then have the generic Entity compute the id as the hash of the body's serialization.
If we don't, like we do here (because recipient and anchor block are not included) then we should definitely explain what it means for there to be multiple entities with the same id and different recipients / anchor blocks.
(incidentally I think we would not actually support that? Because the entity store only checks for pre-existence of the entity id and then does nothing if there's a match, regardless of content. So e.g. same message for multiple recipients would result in the latter ones being dropped)
| set_contract_sync_cache_invalid(contract_address, messages.map(|msg| msg.recipient)); | ||
| } | ||
|
|
||
| /// Step function to advance the reception workflow of an offchain message. |
There was a problem hiding this comment.
It took me a while to understand this function, including reasoning about the correctness of the TTL. I feel like there's an explanation missing of what this entity and workflow are supposed to do, how they operate and how they handle edge cases, from which the FSM states and transitions would be derived and therefore the implementation of this function.
There was a problem hiding this comment.
In particular, I don't understand how tx-less messages are handled. The logic seems to be as follows:
A message contains a payload and timestamp, and may be related to a transaction. A message is processed only once, when we see the transaction (this is a fact). (notably tx less messages seem to never get processed)
An unprocessed message expires after the TTL. This is because a transaction known at a time T could not possibly begin to exist after the TTL, because this is larger than the maximum transaction expiration duration (which is not mentioned), therefore it is safe to stop looking for it. We add a 2 hour safety margin.
A processed message also expires after the TTL. This is because we're being a bit lazy - we could instead wait until finalization of the block associated to the tx that caused us to process it.
There was a problem hiding this comment.
The comment below explains some of these things, but I feel like the explanation already assumes you understand parts of how this works, and it also acts on a sort of 'collapsed' view of the FSM (e.g. the combination of the processed and unprocessed termination conditions, etc.).
| // TODO(@mverzilli): we could annotate retractable entities and facts as "final" when their corresponding blocks | ||
| // are below the final tip. Then we could use that as termination condition instead of relying on timestamp | ||
| // arithmetics. |
There was a problem hiding this comment.
Perhaps what we could do is have PXE augment the facts with the status of their origin block, notably whether they're finalized or not. We already have this information in PXE, there's no need for a network roundtrip.
| // The "problem" with that is that we would need some sort of heuristic to cleanup. Maybe the whole | ||
| // retractable/non-retractable distinction is useless at the entity level. We can simply handle entities and | ||
| // if they are started with a block, they are retracted with a block; if they are terminated with a block, they | ||
| // are "unterminated" with a block, but also are removed when a block is final. | ||
| // This, in offchain reception's particular case, would make facts entirely unnecessary (at least at its present | ||
| // functionality). |
There was a problem hiding this comment.
I think the building blocks are fine for now, I'd not worry about this. If anything I'd try to embrace the abstraction and see where that takes us instead of trying to optimize it away because we can manage with fewer building blocks in this case.
| reception.body.tx_hash.unwrap_or(0) | ||
| })); | ||
|
|
||
| assert_eq(resolved_txs.len(), active_receptions.len()); |
There was a problem hiding this comment.
I think this is a PXE guarantee? Breakage here would be a bug, this looks more like a test for get_resolved_txs.
| // Ask PXE to resolve each message's originating tx. We pass the tx hashes in active-reception order, so the | ||
| // resolved txs come back aligned with the reception indices and can be matched back positionally. | ||
| let resolved_txs = get_resolved_txs(active_receptions.map(|reception: Entity<OffchainMessage>| { | ||
| reception.body.tx_hash.unwrap_or(0) |
There was a problem hiding this comment.
I see now where the 0 hash in a past PR orignated from. This should be an option, with an explanation of why it makes sense to have optional queries (which is obvious here but very non-obvious in TS).
Re-implementation of offchain reception workflows using the new EntityStore machinery
Note this exposed a problem in the EntityStore idempotency semantics: throwing on an attempt at creating an already existing entity is problematic, because Noir doesn't provide error handling mechanisms. In the end, I decided to make entity creation idempotent, with the first attempt at creating a given entity "winning" to avoid inadvertent overwrites. It is then user responsibility to model their workflows properly (there's plenty of alternatives: change how entity id's are derived, check for existence before attempting creation, using facts, etc).
Closes F-682